-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Korge memory split #2170
Korge memory split #2170
Conversation
private var _size: Int = size | ||
var _size: Int = size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the problem with the half structure. If I do not expose code like this, I can't make those functions extention functions. This was my problem in the original PR. I need some help with it, because I don't know how the Half stuff works and why.
private fun ensureCount(count: Int) { | ||
fun ensureCount(count: Int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
fun ByteArrayBuilder.f16(v: Half, little: Boolean): ByteArrayBuilder = this.apply { prepare(2) { data.setF16(_size, v, little) } } | ||
fun ByteArrayBuilder.f16LE(v: Half): ByteArrayBuilder = this.apply { prepare(2) { data.setF16LE(_size, v) } } | ||
fun ByteArrayBuilder.f16BE(v: Half): ByteArrayBuilder = this.apply { prepare(2) { data.setF16BE(_size, v) } } | ||
|
||
fun ByteArrayBuilderLE.f16(v: Half): ByteArrayBuilder = bab.f16LE(v) | ||
fun ByteArrayBuilderBE.f16(v: Half): ByteArrayBuilder = bab.f16BE(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public inline class Half(public val rawBits: UShort) { |
We can use s16
and Half.rawBits
to expose these methods without exposing the private functionality.
For example:
fun ByteArrayBuilder.f16(v: Half, little: Boolean): ByteArrayBuilder = s16(v.rawBits, little)
Analogously the rest of the methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! This solves it, thanks!
fun ByteArrayReader.f16(little: Boolean): Half = move(2) { getF16(it, little) } | ||
fun ByteArrayReader.f16LE(): Half = move(2) { getF16LE(it) } | ||
fun ByteArrayReader.f16BE(): Half = move(2) { getF16BE(it) } | ||
fun ByteArrayReaderLE.f16(): Half = bar.f16LE() | ||
fun ByteArrayReaderBE.f16(): Half = bar.f16BE() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To not expose move
:
fun ByteArrayReader.f16(little: Boolean): Half = Half.fromBits(s16(little))
// ...
So in general, we use s16
(Short
type) methods, and convert back and forth with Half.fromBits
and half.rawBits.toShort()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with extracting the Half
functionality from memory, to not add that extra dependency. But private functionaluty shouldn't be exposed in the process.
As commented Half.rawBits.toShort
and Half.fromBits
using with s16
variants should do the trick.
commonMainApi(libs.kotlinx.coroutines.core) | ||
commonMainApi(libs.kotlinx.atomicfu) | ||
commonTestApi(libs.kotlinx.coroutines.test) | ||
commonMainApi(project(":korlibs-math-core")) | ||
commonMainApi(project(":korlibs-platform")) | ||
commonMainApi(project(":korlibs-util")) | ||
commonMainApi(project(":korlibs-crypto")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should evaluate those.
- kotlinx-coroutines are not used at all
- math-core might be used, but maybe we can internally copy stuff to remove that dependency?
- util and crypto probably not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util and math-core are used. We should avoid copiing stuff to private. I check the other two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's keep it as it is for now. We can optimize dependencies later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not remove any of them
private fun ByteArray.u8(offset: Int): Int = this[offset].toInt() and 0xFF | ||
private inline fun ByteArray.get16LE(offset: Int): Int = (u8(offset + 0) shl 0) or (u8(offset + 1) shl 8) | ||
private inline fun ByteArray.get16BE(offset: Int): Int = (u8(offset + 1) shl 0) or (u8(offset + 0) shl 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these exposed publicly already in other file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know about that. They were private in the original class
Thanks! |
No description provided.